fix(runtime-host): classify archived Skill queries - #5149
Conversation
8dfa368 to
47dad67
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
The design is right and the Host-side fix is the authoritative one. skill.catalog.invocable.query was the only session-targeted operation that collapsed an archived Session into internal_failure — AgentGraphCoordinator#assertRootSupervisor throws session_archived (packages/runtime/src/stream-graph-coordinator.ts:1331) and repositoryFailure had no case for it; the new header check mirrors plan-coordinator.ts:280-296. I reproduced #4430 on the base commit and all three new tests go red there.
Two gaps before merge.
P2 — not_found still collapses to internal_failure on the same resolver (reachability ②)
packages/runtime-host/src/server/execution-composition.ts:716 reads the header before the new archived check. A removed Session makes readHeaderSnapshot throw SessionNotFoundError, which repositoryFailure (skill-catalog-coordinator.ts:279-290) turns into internal_failure — #4430 again, sibling condition. session.remove is supported (protocol/operations.ts:327): a renderer holds a sessionId, another window removes the Session, composer-mentions.tsx refreshes on the sessions event and queries a Session that is gone. plan-coordinator.ts:284-295 handles both in one place; splitting them costs a second compatibility-epoch bump later.
Fix: add 'not_found' to INVOCABLE_QUERY_ERRORS (protocol/skill-catalog.ts:54) and wrap the header read:
let header;
try {
header = await stores.sessionStore.readHeaderSnapshot(sessionId);
} catch (error) {
if (isSessionNotFoundError(error)) {
throw new SkillCatalogInvocableContextError('not_found', 'Session does not exist');
}
throw error;
}isSessionNotFoundError is already imported at execution-composition.ts:117; the error code widens to 'session_archived' | 'not_found'.
P2 — the desktop projection has no observable effect (reachability ①)
apps/desktop/src/main/runtime-host-skills-ipc-main.ts:127-139 catches session_archived and returns []. Its only consumer, composer-mentions.tsx:175, already produces the same state on rejection (:194-200): { loading: false, settled: 'empty', skills: EMPTY_SKILLS }. A session_archived rejection is also not an isReconnectableReadFailure (apps/desktop/src/main/ipc-reconnect-policy.ts:85-94), so no reconnect or retry either — both paths give the same empty / menu.
So 14 production lines, the new @maka/runtime-host/client main-process import and 60 test lines assert an unobservable difference. Name the observable difference in the body, or drop the desktop change and keep the PR to the Host contract plus the epoch bump; #4431 already gates the renderer.
P3 — PR body states the wrong epoch
The body says 139 → 140; the diff moves 141 → 142 (packages/runtime-host/src/protocol/index.ts:104). Stale from a rebase, and peers key on the epoch number.
Verification I ran
npm ci + npm run build at the PR head: runtime-host execution-composition.test.js + skill-catalog-protocol.test.js 40 pass, apps/desktop runtime-host-skills-ipc-main.test.js 4 pass; with the five files reverted to origin/main, the three new tests go red (internal_failure on the archived case) while preserves non-archival invocable Skill failures still passes.
中文
设计方向正确,Host 端的修法是权威修法。skill.catalog.invocable.query 是唯一把已归档 Session 折叠成 internal_failure 的会话级操作——AgentGraphCoordinator#assertRootSupervisor 抛 session_archived(packages/runtime/src/stream-graph-coordinator.ts:1331),而 repositoryFailure 没有对应分支;新增的 header 检查与 plan-coordinator.ts:280-296 一致。我在 base commit 上复现了 #4430,三个新测试在 base 上都是红的。
合并前还有两个缺口。
P2 — 同一 resolver 上 not_found 仍折叠成 internal_failure(可达类别 ②)
packages/runtime-host/src/server/execution-composition.ts:716 在新增的归档检查之前读 header。Session 已被删除时 readHeaderSnapshot 抛 SessionNotFoundError,被 repositoryFailure(skill-catalog-coordinator.ts:279-290)变成 internal_failure——还是 #4430,只是姐妹条件。session.remove 是受支持的操作(packages/runtime-host/src/protocol/operations.ts:327):renderer 持有 sessionId,另一窗口删除该 Session,composer-mentions.tsx 收到 sessions 事件刷新,去查一个已不存在的 Session。plan-coordinator.ts:284-295 在同一处处理两者;拆开做意味着以后要为这个 PR 已经在改的条件再加一次兼容 epoch。
修法:在 INVOCABLE_QUERY_ERRORS(protocol/skill-catalog.ts:54)加上 'not_found',并包住 header 读取:
let header;
try {
header = await stores.sessionStore.readHeaderSnapshot(sessionId);
} catch (error) {
if (isSessionNotFoundError(error)) {
throw new SkillCatalogInvocableContextError('not_found', 'Session does not exist');
}
throw error;
}isSessionNotFoundError 已在 execution-composition.ts:117 导入;错误 code 扩为 'session_archived' | 'not_found'。
P2 — desktop 的投影没有可观察效果(可达类别 ①)
apps/desktop/src/main/runtime-host-skills-ipc-main.ts:127-139 捕获 session_archived 并返回 []。它唯一的消费者 apps/desktop/src/renderer/composer-mentions.tsx:175 在拒绝分支(:194-200)已产生同样的状态:{ loading: false, settled: 'empty', skills: EMPTY_SKILLS }。session_archived 的拒绝也不属于 isReconnectableReadFailure(apps/desktop/src/main/ipc-reconnect-policy.ts:85-94),不触发重连或重试——两条路径都落到同一个空的 / 菜单。
也就是说,14 行产品代码、主进程新增的 @maka/runtime-host/client 导入和 60 行测试,断言的是观察不到的差异。请在 PR 正文说清可观察差异,或去掉 desktop 改动、把 PR 收敛到 Host 契约加 epoch 提升;#4431 已经在 renderer 侧做了门控。
P3 — PR 正文的 epoch 写错
正文写 139 → 140,diff 实际是 141 → 142(packages/runtime-host/src/protocol/index.ts:104)。应是 rebase 后没更新,而对端就是按 epoch 数字判断的。
我跑过的验证
在 PR head 上 npm ci + npm run build:runtime-host 的 execution-composition.test.js + skill-catalog-protocol.test.js 40 通过,apps/desktop 的 runtime-host-skills-ipc-main.test.js 4 通过;把五个文件回退到 origin/main 后,三个新测试变红(归档用例以 internal_failure 失败),而 preserves non-archival invocable Skill failures 仍通过。
Invocable Skill discovery flattened missing and archived Session lifecycle refusals into internal_failure. Preserve not_found and session_archived through the protocol, including lifecycle changes during live graph-tool resolution. Fixes apache#5148 Generated-by: OpenAI Codex Signed-off-by: liuxiaocs7 <liuxiao2103@qq.com>
98aa255 to
4f95e41
Compare
|
Rebased onto the latest
Verification:
Both inline comments have been answered and resolved. |
Astro-Han
left a comment
There was a problem hiding this comment.
All five points from the previous round are addressed, and the result is smaller than what I asked for: the desktop projection is gone, the lifecycle mapping lives next to the resolver that raises it, and repositoryFailure stays generic for the other three catalog operations. not_found and session_archived land in the same wire union under one epoch bump, so there is no second compatibility break later.
| Previous finding | Status |
|---|---|
P2 — not_found collapses to internal_failure |
Addressed (execution-composition.ts:716-724, skill-catalog.ts:54), and extended to the removal-mid-resolve race |
| P2 — desktop projection has no observable effect | Addressed — desktop change dropped entirely; diff is six Runtime Host files |
| P3 — PR body states the wrong epoch | Addressed — body and protocol/index.ts:104 both read 141 → 142 |
P3 (inline) — repositoryFailure cast |
Addressed — dedicated onFailure mapper for queryInvocable; shared path unchanged vs main |
| P3 (inline) — archive race untested | Addressed — both race cases added, and they are load-bearing (see below) |
Nothing new at P0–P2. One P3 left inline on the archived pre-check.
Verification I ran
Built @maka/runtime-host at head: execution-composition.test.js + skill-catalog-protocol.test.js 43 pass. Ablations:
- Reverting the four production files to
origin/main: all four new composition tests and the protocol decode case go red (5 failures). - Keeping the header pre-checks but deleting only the
catcharoundrunWithSessionBindingPreview(execution-composition.ts:735-749): the two race tests go red, the two pre-check tests stay green. The race coverage is real, not incidental. - Deleting only the
header.isArchivedpre-check (:725-727): all four stay green — that is the P3 below.
I did not run the desktop suite; apps/desktop is now byte-identical to main.
中文
上一轮的五条都已处理,而且结果比我要求的更小:desktop 投影整个去掉,生命周期映射放在抛出它的 resolver 旁边,repositoryFailure 对其余三个目录操作保持通用。not_found 和 session_archived 落在同一个 wire 错误集合、共用一次 epoch 提升,后续不会再来一次兼容性断裂。
| 上一轮的意见 | 状态 |
|---|---|
P2 — not_found折叠成 internal_failure |
已处理(execution-composition.ts:716-724、skill-catalog.ts:54),并扩展到解析中删除的竞态 |
| P2 — desktop 投影没有可观察效果 | 已处理——desktop 改动完全去掉,diff 只剩六个 Runtime Host 文件 |
| P3 — PR 正文 epoch 写错 | 已处理——正文与 protocol/index.ts:104 均为 141 → 142 |
P3(inline)— repositoryFailure 的 cast |
已处理——queryInvocable 有专用 onFailure 映射,共用路径相对 main 未改 |
| P3(inline)— 归档竞态没有测试 | 已处理——两个竞态用例都补上,且确实起作用(见下) |
没有新的 P0–P2。归档前置检查上留了一条 P3 inline。
我跑过的验证
在 head 上构建 @maka/runtime-host:execution-composition.test.js + skill-catalog-protocol.test.js 43 通过。消融:
- 把四个产品文件回退到
origin/main:四个新的 composition 测试和协议解码用例全部变红(5 个失败)。 - 保留 header 前置检查、只删掉
runWithSessionBindingPreview外面的catch(execution-composition.ts:735-749):两个竞态测试变红,两个前置检查测试仍绿。竞态覆盖是真实的,不是顺带通过的。 - 只删掉
header.isArchived前置检查(:725-727):四个测试全绿——即下面那条 P3。
我没有跑 desktop 套件;apps/desktop 现在与 main 完全一致。
| } | ||
| throw error; | ||
| } | ||
| if (header.isArchived) { |
There was a problem hiding this comment.
Deleting these three lines leaves all four new tests green, so nothing pins this pre-check — yet it is not redundant with the catch below: resolveAvailableToolNames returns early for subagent headers (:1097-1116) without ever reaching AgentGraphCoordinator#assertRootSupervisor, so an archived linked child Session is refused only here. Assert that too — a case with an archived subagentRuntime header expecting session_archived, or an assertion in the existing archived test that toolsForSession was not called, which is what its name already claims.
删掉这三行后四个新测试仍全绿,但它并不与下面的 catch 重复:subagent header 在 :1097-1116 提前返回,不会走到 assertRootSupervisor,已归档的子 Session 只靠这里拒绝。建议补一例断言,或在现有归档用例里断言未解析实时工具面(测试名已这么声称)。
Summary
Return typed
not_foundorsession_archivedfailures when invocable Skill discovery targets a missing or archived Session, including removal and archive races during live graph-tool resolution.Keep the lifecycle mapping local to
skill.catalog.invocable.query, so the shared catalog failure path and the other catalog operations retain their existing error contracts. The wire-visible error set moves the compatibility epoch from 141 to 142.This complements the renderer-side automatic-query gate in #4431 while keeping the Runtime Host contract correct for external lifecycle changes, multiple windows, and requests already in flight.
Fixes #5148
Refs #4430
Verification
core,storage,mcp,runtime,runtime-host) — passednpm --workspace @maka/runtime-host run typecheck— passedgit diff --check origin/main— passednpm testremains blocked by pre-existing@maka/uitype errors unrelated to this PRAI use
Select exactly one:
Tool(s) and scope:
OpenAI Codex assisted with diagnosis, implementation, regression tests, verification, and preparing the issue and pull request text under the contributor's direction.
Checklist
Does this PR entail a change in behavior?